Disable caching when publishing built assets#79835
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens release/publishing workflows against cache-poisoning risks by disabling Node/package-manager caching in workflows that build and publish production assets, and by introducing a caching toggle in the repo’s composite Node setup action.
Changes:
- Disable
actions/setup-nodepackage-manager caching in npm publishing and plugin zip build workflows. - Add an
enable-cachinginput to the local.github/setup-nodecomposite action. - Gate the composite action’s post-install-on-cache-hit behavior behind the new
enable-cachinginput.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| .github/workflows/publish-npm-packages.yml | Disables actions/setup-node package-manager caching during npm publishing. |
| .github/workflows/build-plugin-zip.yml | Disables actions/setup-node package-manager caching during plugin zip build/publish steps. |
| .github/setup-node/action.yml | Adds enable-caching input and uses it to influence setup-node caching and post-install behavior. |
Caching can be used when building a zip file for a pull request but should always be disabled for other scenarios (when building the zip for the GitHub Container Registry or for final release).
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @Copilot. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Flaky tests detected in d111a36. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/29345238761
|
…://github.com/WordPress/gutenberg into actions/disable-caching-for-production-builds
It's not clear why this was the only workflow that contains the `cache` input, but removing this will allow `package-manager-cache` only to be used without conflict.
| with: | ||
| node-version-file: ${{ inputs.node-version == '' && '.nvmrc' || '' }} | ||
| node-version: ${{ inputs.node-version }} | ||
| package-manager-cache: ${{ inputs.enable-caching == 'true' && 'true' || 'false' }} |
There was a problem hiding this comment.
Flagged during ai-assisted review
The new guards correctly make the node_modules cache opt-outable, but removing cache: npm also disables actions/setup-node's npm cache for every default caller.
package-manager-cache: true only enables automatic caching when the root package.json declares npm in devEngines.packageManager or the top-level packageManager field (upstream implementation, detection logic). Gutenberg declares neither, so the default enable-caching: true path no longer restores or saves the npm cache across the action's 13 call sites. That contradicts the new input's contract and creates a broad CI performance regression unrelated to the production hardening.
Could we either remove the currently unused composite-action input, or preserve the explicit cache conditionally?
cache: ${{ inputs.enable-caching == 'true' && 'npm' || '' }}
package-manager-cache: false| uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 | ||
| with: | ||
| node-version-file: '.nvmrc' | ||
| package-manager-cache: ${{ matrix.IS_GUTENBERG_PLUGIN == 'true' && github.event_name == 'pull_request' && 'true' || 'false' }} |
There was a problem hiding this comment.
Flagged during ai-assisted review
Keeping npm caching for Gutenberg-plugin PR builds makes sense, but this condition never enables it. matrix.IS_GUTENBERG_PLUGIN comes from the YAML booleans [true, false], while the expression compares it with the string 'true'; under GitHub's loose-equality rules, boolean true is coerced to 1 and the non-numeric string is NaN, so the comparison is false. Even without that mismatch, package-manager-cache: true cannot auto-detect npm because this repository has no packageManager declaration.
Could we make the intended cache input explicit instead, or remove the exception if caching should now stay disabled for all plugin builds?
cache: ${{ matrix.IS_GUTENBERG_PLUGIN && github.event_name == 'pull_request' && 'npm' || '' }}
package-manager-cache: falseThere was a problem hiding this comment.
Yeah, I'd just drop the string comparison ( == 'true') and keep matrix.IS_GUTENBERG_PLUGIN
| uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 | ||
| with: | ||
| node-version-file: '.nvmrc' | ||
| package-manager-cache: ${{ matrix.IS_GUTENBERG_PLUGIN == 'true' && github.event_name == 'pull_request' && 'true' || 'false' }} |
There was a problem hiding this comment.
Yeah, I'd just drop the string comparison ( == 'true') and keep matrix.IS_GUTENBERG_PLUGIN
| required: true | ||
| default: 'true' |
There was a problem hiding this comment.
Why do we have a default for a required input?
|
|
||
| - name: Install npm dependencies | ||
| if: ${{ steps.cache-node_modules.outputs.cache-hit != 'true' }} | ||
| if: ${{ inputs.enable-caching == 'false' || steps.cache-node_modules.outputs.cache-hit != 'true' }} |
There was a problem hiding this comment.
If caching is disabled the cache step is skipped, so cache-hit != true is already true, no?
What?
This disables caching within workflows that build production assets to harden against potential cache poisoning attacks.
Why?
Though it's difficult to achieve, cache poisoning is an attack where release workflows leverage a build state cached from previous workflow executions.
How?
Popular third-party actions configure caching by default but support inputs that can disable the behavior.
Use of AI Tools
None